Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Add checker for formatting style #2613

Closed
wants to merge 1 commit into from

Conversation

0x8000-0000
Copy link
Contributor

No description provided.

@0x8000-0000
Copy link
Contributor Author

Checking all commits, even if a bit wasteful, because GitHub allows submitting a set of patches.

Alternatively, we can compute the merge-base between the HEAD of the pull request and the main branch, then use git-clang-format (but I have found it sometimes unreliable).

@0x8000-0000
Copy link
Contributor Author

Sadly, git-clang-format does not have an equivalent --dry-run flag, to return status 0 if no changes were required an non-0 otherwise. I suppose I could go over to the llvm repo and add that first ;)

@foonathan
Copy link
Contributor

There is a big problem with such a check: clang-format behaves differently in different versions. You're checking for correctly formatted code according to version 11. I have only version 13 installed without an easy way to obtain version 11. I might need to manually reformat code to turn it into something version 11 likes, which defeats the purpose of auto format.

@0x8000-0000
Copy link
Contributor Author

There is a big problem with such a check: clang-format behaves differently in different versions. You're checking for correctly formatted code according to version 11. I have only version 13 installed without an easy way to obtain version 11. I might need to manually reformat code to turn it into something version 11 likes, which defeats the purpose of auto format.

That's fair, but I presume the project will define the reference version for clang-format. I have only picked 11 here because I saw it used in other recipes, so I presumed it is already configured. I suppose if we request Ubuntu 21.10 it would come with Clang13 automatically?

There are several orthogonal problems to solve here:

  • how to run a checker?
  • how to run a checker efficiently
  • which version of the checker to run?

The last question can be answered / tweaked independently of the others. Unless we really need to implement --dry-run in git-clang-format in which case the minimal answer for the clang version is... 14 .

@0x8000-0000
Copy link
Contributor Author

Just to clarify - my preference would be to "live at head" and soon after clang is released, the version of the tool is bumped and a "NFC - reformat with clang-format-$newest" commit be pushed at the same time. But this would require a vote of the most active contributors, since they are most likely to be impacted by the change.

@vitaut
Copy link
Contributor

vitaut commented Nov 25, 2021

@foonathan made a very good point. It would be too much to require contributors install a specific version of clang-format. We could update the PR template to remind them to run clang-format though: https://github.com/fmtlib/fmt/blob/master/.github/pull_request_template.md.

@vitaut vitaut closed this Nov 25, 2021
@vitaut
Copy link
Contributor

vitaut commented Nov 25, 2021

But thanks for the attempt anyway!

@0x8000-0000
Copy link
Contributor Author

@foonathan made a very good point. It would be too much to require contributors install a specific version of clang-format. We could update the PR template to remind them to run clang-format though: https://github.com/fmtlib/fmt/blob/master/.github/pull_request_template.md.

"Never send a human to do a machine's job!" - is this from one of the Terminator movies?

I suppose we could live with a periodic "clean-up accumulated lint".

Most of the contributors here are sophisticated C++ engineers, surely they can install the latest LLVM binaries to run the tool? Otherwise how are they going to follow the request in pull_request_template?

@vitaut
Copy link
Contributor

vitaut commented Nov 25, 2021

To clarify: I was not suggesting to ask users to install a specific version in the PR template, just run whatever version they have. We can live with minor discrepancies but don't want to make contributing unnecessarily complicated.

@0x8000-0000
Copy link
Contributor Author

To clarify: I was not suggesting to ask users to install a specific version in the PR template, just run whatever version they have. We can live with minor discrepancies but don't want to make contributing unnecessarily complicated.

Fair enough - thank you for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants